Skip to content

Conversation

@Blendify
Copy link
Member

@Blendify Blendify commented Aug 9, 2021

This likely was indirectly broken with da86e4d

This commit addes our standard headerlink icon styles to math links aswell, before they had no styling.

Fixes #1189

This likely was indirectly broken with da86e4d

This commit addes our standard headerlink icon styles to math links aswell, before they had no styling.

Fixes #1189
@Blendify Blendify requested review from a team, agjohnson and nienn August 9, 2021 21:47
@Blendify Blendify added this to the 1.0 milestone Aug 9, 2021

// This is the #href that shows up on hover. Sphinx's is terrible so I hack it away.
h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption
h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption, .eqno
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CSS selector seems a bit too specific in the future we may want to use just .headerlink

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this list is just for applying the opacity on the parent element, otherwise we might be able to use .headerlink, yeah. This seems fine for now.

We could also use the entire .math parent element instead here, so that hovering over the math element will give the anchor link. I'm not sure how folks normally use the equation number here, but it might be hard to discover this anchor link if the hover event is just on the equation number.

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better! It is a bit hard to find the link to the equation now, though not sure how useful the link was before. I could see however on the parent .math class working well too.


// This is the #href that shows up on hover. Sphinx's is terrible so I hack it away.
h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption
h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption, .eqno
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this list is just for applying the opacity on the parent element, otherwise we might be able to use .headerlink, yeah. This seems fine for now.

We could also use the entire .math parent element instead here, so that hovering over the math element will give the anchor link. I'm not sure how folks normally use the equation number here, but it might be hard to discover this anchor link if the hover event is just on the equation number.

@Blendify
Copy link
Member Author

Looks better! It is a bit hard to find the link to the equation now, though not sure how useful the link was before. I could see however on the parent .math class working well too.

I agree this is something I noticed but wasn't sure if we want to go out of the scope of the bugfix here.
One solution I found was to make eqno display: block which makes the icon appear when hovering anywhere next to the headerlink.

Making it appear when covering the whole math equation is also a solution though. Maybe this should be tackled in 1.1 as part of: #383

@nienn
Copy link
Contributor

nienn commented Aug 10, 2021

This looks good!

Also :bump to # 383 as part of 1.1.

@agjohnson
Copy link
Collaborator

Sounds good, let's address it more in 1.1

@agjohnson agjohnson merged commit f0e90ac into master Aug 12, 2021
@agjohnson agjohnson deleted the Blendify/fix-math-link-icon branch August 12, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing font on mathjax output

4 participants